Ensure session expirations are Dates #132
Conversation
paulomarg
force-pushed
the
ensure_session_expires_is_date
branch
from
ac0bc56
to
5da6add
Compare
| let session = new Session(result.id as string); | ||
| session = {...session, ...result}; | ||
|
|
||
| if (session.expires && typeof session.expires === 'string') { | ||
| session.expires = new Date(session.expires); | ||
| } |
There was a problem hiding this comment.
| let session = new Session(result.id as string); | |
| session = {...session, ...result}; | |
| if (session.expires && typeof session.expires === 'string') { | |
| session.expires = new Date(session.expires); | |
| } | |
| const session = Session.cloneSession(result, result.id); |
and following part port to Session.cloneSession.
if (session.expires && typeof session.expires === 'string') {
session.expires = new Date(session.expires);
}There was a problem hiding this comment.
Hmm, I don't think that would work - we can't guarantee that result will be of type Session here, since it may have been loaded from something like a JSON.parse() call.
If we tried to call Session.clone with result it would break in Typescript, and we shouldn't expect the expires field to be a string there (it is typed as Date so it would have to be set when the session is created anyway).
There was a problem hiding this comment.
@paulomarg I got it.
My suggestion is for the following reasons:
I wish I could improve the point that users have to convert the session.expires to Date type.
(This seems to be solved if session.isActive() is implemented)
session = {...session, ...result}; overwrites session.id with result.id.
If this assignment is OK, why does Session.cloneSession exist?
paulomarg
force-pushed
the
ensure_session_expires_is_date
branch
from
5da6add
to
240a8e7
Compare
WHY are these changes introduced?
It was pointed out (Shopify/koa-shopify-auth#65 (comment)) that the
CustomSessionStorageclass may cause issues if the returned sessionexpiresfield isn't aDateobject.WHAT is this pull request doing?
Making sure that we allow strings to be returned for the expiration date, and handling the conversion to
Date.Type of change
Checklist
I have documented new APIs/updated the documentation for modified APIs (for public APIs)